Convert to xarray#935
Conversation
davidhassell
added
enhancement
|
(Removing NETXVERSION label since we agreed this won't make it in for today's release.) |
|
(Now adding the new NEXTVERSION milestone, after changing the old one to 3.20.0...) |
|
(Now reviewing, so have went in to resolve the trivial merge conflicts on the changelog, which meant bumping the changelog lines added in this PR to a new 'NEXTVERSION' release entry, because since this PR was raised we have made that release and bumped this one back.) |
There was a problem hiding this comment.
Excellent. +1 in particular for the thorough testing (though see point (3) below).
A few minor things e.g. a (repeated) typo, but otherwise ready to merge. Though before you do, some notes:
-
We might want to consider bumping the minimum version pinned for
xarrayof2026.2.0given that it has been some months since you put up the PR up such that xarray is now at2026.7.0. But either is fine so that's up to you. -
I noticed in reviewing that the
DomainandDomainListclasses aren't clickable in the docs (at least for the former via the mini table at https://ncas-cms.github.io/cf-python/class.html#domain-class). The latter doesn't seem to be listed under../docs/source/classso I think it needs a stub file added there. (It would be interesting to read the docs on the latter because though I knew aFieldListwas a thing I wasn't aware we had aDomainListtoo!)As for the former (
Domain), are aware of some reason why that might not be clickable (otherwise will likely be some issue with the docs build I didn't notice at release time) since there is a stub filecf.Domain.rst? -
Regarding testing, my comment from the sister
cfdmPR applies here too, and they could be tackled at the same time since it is essentially the same testing:That said, we should definitely work towards more comprehensive testing than just asking 'is it a valid xarray dataset/tree?' after conversion to confirm that the conversion maps everything correctly,
Yes - moved to the latest: 1304436 |
I propose we park this until we also have "from xarray" - then we can test the round-trip of cf.Field -> xr.Dataset -> cf.Field. I suspect that the round trip won't be perfect, but we can easily test the before and after FIelds for where we expect them to match. If you agree, I'll put up an issue so that we don't forget. |
Good catch - would you like to put up an issue? |
Nice, though it is also referenced in the
Yes, good point and that's a great plan. Agreed.
Will do! I think this just needs a bit extra for the first point above and then we're good to go. And then we both have a follow-on Issue to write up 🙂 |
|
xarray version fixed: e7af4b5 |
2 participants
Fixes #933
Needs #922 to be merged, first.- which it now is!